fix($compile): support merging special attribute names in `replace` d… #13318

petebacondarwin · 2015-11-16T22:30:08Z

…irectives

When compiling a replace directive, the compiler merges the attributes from
the replaced element onto the template element.

Unfortunately, setAttribute and other related DOM methods do not allow certain
attribute names - in particular Angular 2 style names such as (click) and [value].

This is relevant when using ngForward with Angular Material, since the mgButton
directive uses replace and in the former you often use (click).

This fixes the problem but for those special attributes the speed is considerably
slow.

Closes #13317

gkalpak · 2015-11-17T11:56:28Z

src/ng/compile.js

@@ -1221,6 +1228,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
      }
    };

+    function setSpecialAttr(element, attrName, value) {
+      specialAttrHolder.innerHTML = "<span " + attrName + "='" + (value || '').replace(QUOTE_REGEX, '&quot;') + "'>";
+      var attribute = specialAttrHolder.firstChild.attributes[0].cloneNode();


According to MDN, cloneNode() is supposed to be deprecated.
_{It also says that, since Chrome v45, `Attr` no longer inherits from `Node`, but `cloneNode()` seems to still work on Chrome v46.}

Yeah, trouble is I can't find any other way of achieving the desired solution. Any other ideas?

Ahah how about this:

specialAttrHolder.innerHTML = "<span " + attrName + "='" + (value || '').replace(QUOTE_REGEX, '"') + "'>" var span = specialAttrHolder.firstChild; var attribute = span.attributes[0]; span.removeAttribute(attrName); element.attributes.setNamedItem(attribute);

This avoids any deprecated API and could possibly be a lot quicker as we are not cloning anything.

…irectives When compiling a `replace` directive, the compiler merges the attributes from the replaced element onto the template element. Unfortunately, `setAttribute` and other related DOM methods do not allow certain attribute names - in particular Angular 2 style names such as `(click)` and `[value]`. This is relevant when using ngForward with Angular Material, since the `mgButton` directive uses `replace` and in the former you often use `(click)`. This fixes the problem but for those special attributes the speed is considerably slow. Closes angular#13317 Closes angular#13318

timkindberg · 2015-11-17T18:09:41Z

so this performance hit will only happen in certain scenarios? It will not happen for everything using ng-forward special attributes correct?

jbedard · 2015-11-17T18:14:39Z

src/ng/compile.js

@@ -989,6 +989,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
    function($injector,   $interpolate,   $exceptionHandler,   $templateRequest,   $parse,
             $controller,   $rootScope,   $document,   $sce,   $animate,   $$sanitizeUri) {

+    var SIMPLE_ATTR_NAME = /^\w/;
+    var QUOTE_REGEX = /'/g;


This looks unused...

Oh yes. That was from a previous commit in this pr

petebacondarwin · 2015-11-17T18:46:27Z

@jbedard yes. It has no impact for almost all circumstances

jbedard · 2015-11-17T19:12:57Z

That wasn't me asking, but good to know :)

Using innerHTML looks painful but I don't see any other way. Except maybe convincing people to not use replace:true?

petebacondarwin · 2015-11-17T21:17:18Z

Sorry @jbedard - difficult to keep track on a phone.
@timkindberg - yes you are correct. This only occurs if replace: trueis used on a directive AND the user of the directive uses "special" attributes such as (click) or [value]. If ng-forward supported the long form of these attributes, such as bind-value and on-click then the user can always choose to use that form instead on directives that are replace: true and not incur any performance degradation.

I spent ages digging through the DOM API and hacking in different browsers. The final solution we have here is about as fast as I can get and doesn't use any deprecated APIs. But if anyone has time to find a faster solution that would be great.

I'm afraid that we are stuck with replace: true for the time being...

…irectives When compiling a `replace` directive, the compiler merges the attributes from the replaced element onto the template element. Unfortunately, `setAttribute` and other related DOM methods do not allow certain attribute names - in particular Angular 2 style names such as `(click)` and `[value]`. This is relevant when using ngForward with Angular Material, since the `mgButton` directive uses `replace` and in the former you often use `(click)`. This fixes the problem but for those special attributes the speed is considerably slow. Closes angular#13317 Closes angular#13318

lgalfaso · 2015-11-17T22:53:30Z

@petebacondarwin I wonder if we really have to support this. Would it be possible that if someone needs this, then they have to use the extended notation? My rational is that building html by concatenating strings enlarge the surface for mXSS.

petebacondarwin · 2015-11-18T09:55:38Z

@lgalfaso I see where you are coming from about security.

I think that in this case we are safe though (please do check the accuracy of this):

we are creating a single div containing a span, which is never connected to the main document. Any dubious HTML that is injected into this div's innerHTML will never be executable. (Is that correct?)
we are using DOM manipulation (span.attributes[0]) to get hold of exactly the attribute object that we created in the innerHTML. Even if additional HTML was injected into the innerHTML it could not bleed through into the destination element.

lgalfaso · 2015-11-18T10:53:05Z

@petebacondarwin this solution still has issues with attributes with special names (tried this with Chrome) http://plnkr.co/edit/0GjwipSakMskljc7X3Bl?p=preview

Just run it and check the console

petebacondarwin · 2015-11-18T12:02:45Z

@lgalfaso - thanks for spotting that. We have to use the attributes.removeNamedItem(attribute.name) to get it to work properly. Other combinations seem to fail for one reason or another.

…irectives When compiling a `replace` directive, the compiler merges the attributes from the replaced element onto the template element. Unfortunately, `setAttribute` and other related DOM methods do not allow certain attribute names - in particular Angular 2 style names such as `(click)` and `[value]`. This is relevant when using ngForward with Angular Material, since the `mgButton` directive uses `replace` and in the former you often use `(click)`. This fixes the problem but for those special attributes the speed is considerably slow. Closes angular#13317 Closes angular#13318

petebacondarwin · 2015-11-18T12:11:34Z

Added a test and a fix for the problem that @lgalfaso noted

…irectives When compiling a `replace` directive, the compiler merges the attributes from the replaced element onto the template element. Unfortunately, `setAttribute` and other related DOM methods do not allow certain attribute names - in particular Angular 2 style names such as `(click)` and `[value]`. This is relevant when using ngForward with Angular Material, since the `mgButton` directive uses `replace` and in the former you often use `(click)`. This fixes the problem but for those special attributes the speed is considerably slow. Closes angular#13317

…irectives When compiling a `replace` directive, the compiler merges the attributes from the replaced element onto the template element. Unfortunately, `setAttribute` and other related DOM methods do not allow certain attribute names - in particular Angular 2 style names such as `(click)` and `[value]`. This is relevant when using ngForward with Angular Material, since the `mgButton` directive uses `replace` and in the former you often use `(click)`. This fixes the problem but for those special attributes the speed is considerably slow. Closes angular#13317 Closes angular#13318

lgalfaso · 2015-11-18T17:01:05Z

@petebacondarwin can you please remove all the console.log?

petebacondarwin · 2015-11-18T17:38:15Z

The console log are there because there was a failure on Firefox bit I couldn't reproduce it locally. Try pulling the previous commit for the proper solution

petebacondarwin · 2015-11-18T18:32:10Z

Well this is interesting...

Given

<h1 Ω="omega">Hello Plunker!</h1>

the following fails only with JQuery on Firefox

var h1 = $('h1');
expect(h1.attr('Ω')).toEqual('omega');

http://plnkr.co/edit/qrmsTZsW78CYxJSi7Pyv?p=preview

compared to with jqLite:

http://plnkr.co/edit/h9Zfji8HtgcFnGKF49My?p=preview

Is that a bug in JQuery @mzgol?

mgol · 2015-11-18T18:55:16Z

Hmm, interesting, native getAttribute works. I'll need to look at it closer. (not today, though)

mgol · 2015-11-18T19:03:05Z

@petebacondarwin Seems like a bug, I reported it to the jQuery bug tracker: jquery/jquery#2730

mgol · 2015-11-18T19:22:00Z

OK, please read jquery/jquery#2730 (comment). So, jQuery is using String#lowercase on the input which is incorrect according to the spec (citations in the linked comment) but the reason it breaks only in Firefox is that only Firefox is apparently following the spec here...

…irectives When compiling a `replace` directive, the compiler merges the attributes from the replaced element onto the template element. Unfortunately, `setAttribute` and other related DOM methods do not allow certain attribute names - in particular Angular 2 style names such as `(click)` and `[value]`. This is relevant when using ngForward with Angular Material, since the `mgButton` directive uses `replace` and in the former you often use `(click)`. This fixes the problem but for those special attributes the speed is considerably slow. Closes angular#13317 Closes angular#13318

googlebot added the cla: yes label Nov 16, 2015

petebacondarwin added this to the 1.5.0-beta.2 milestone Nov 17, 2015

petebacondarwin added type: bug component: $compile needs: review severity: inconvenient branch: 1.5.x labels Nov 17, 2015

gkalpak reviewed Nov 17, 2015
View reviewed changes

jbedard reviewed Nov 17, 2015
View reviewed changes

matsko modified the milestones: 1.5.0-beta.2, 1.5.0-beta.3 Nov 18, 2015

petebacondarwin added 5 commits November 18, 2015 14:17

petebacondarwin force-pushed the issue-13317 branch from 78eb562 to c93d5cf Compare November 18, 2015 14:17

TESTING

b0fda7f

petebacondarwin added 2 commits November 18, 2015 17:54

TESTING2

74bf194

TESTING3

ee8e22a

mgol mentioned this pull request Nov 18, 2015

.attr() getter doesn't accept some characters in Firefox jquery/jquery#2730

Closed

petebacondarwin closed this in a5ff651 Nov 19, 2015

petebacondarwin deleted the issue-13317 branch November 24, 2016 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($compile): support merging special attribute names in `replace` d… #13318

fix($compile): support merging special attribute names in `replace` d… #13318

petebacondarwin commented Nov 16, 2015

gkalpak Nov 17, 2015

petebacondarwin Nov 17, 2015

petebacondarwin Nov 17, 2015

timkindberg commented Nov 17, 2015

jbedard Nov 17, 2015

petebacondarwin Nov 17, 2015

petebacondarwin commented Nov 17, 2015

jbedard commented Nov 17, 2015

petebacondarwin commented Nov 17, 2015

lgalfaso commented Nov 17, 2015

petebacondarwin commented Nov 18, 2015

lgalfaso commented Nov 18, 2015

petebacondarwin commented Nov 18, 2015

petebacondarwin commented Nov 18, 2015

lgalfaso commented Nov 18, 2015

petebacondarwin commented Nov 18, 2015

petebacondarwin commented Nov 18, 2015

mgol commented Nov 18, 2015

mgol commented Nov 18, 2015

mgol commented Nov 18, 2015

fix($compile): support merging special attribute names in replace d… #13318

fix($compile): support merging special attribute names in replace d… #13318

Conversation

petebacondarwin commented Nov 16, 2015

gkalpak Nov 17, 2015

Choose a reason for hiding this comment

petebacondarwin Nov 17, 2015

Choose a reason for hiding this comment

petebacondarwin Nov 17, 2015

Choose a reason for hiding this comment

timkindberg commented Nov 17, 2015

jbedard Nov 17, 2015

Choose a reason for hiding this comment

petebacondarwin Nov 17, 2015

Choose a reason for hiding this comment

petebacondarwin commented Nov 17, 2015

jbedard commented Nov 17, 2015

petebacondarwin commented Nov 17, 2015

lgalfaso commented Nov 17, 2015

petebacondarwin commented Nov 18, 2015

lgalfaso commented Nov 18, 2015

petebacondarwin commented Nov 18, 2015

petebacondarwin commented Nov 18, 2015

lgalfaso commented Nov 18, 2015

petebacondarwin commented Nov 18, 2015

petebacondarwin commented Nov 18, 2015

mgol commented Nov 18, 2015

mgol commented Nov 18, 2015

mgol commented Nov 18, 2015

fix($compile): support merging special attribute names in `replace` d… #13318

fix($compile): support merging special attribute names in `replace` d… #13318